Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Implement EIP-2565 (option B) #11728

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Implement EIP-2565 (option B) #11728

wants to merge 1 commit into from

Conversation

vorot93
Copy link

@vorot93 vorot93 commented May 21, 2020

This PR implements EIP-2565: Repricing of the EIP-198 ModExp precompile, more specifically Option 2. It introduces a new computational complexity formula which should better align with the real computational costs for OpenEthereum and Geth.

No test vectors yet, so still a draft PR.

@vorot93 vorot93 added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels May 21, 2020
@vorot93 vorot93 self-assigned this May 21, 2020
@dvdplm
Copy link
Collaborator

dvdplm commented May 22, 2020

By "Option B", do you mean option 2?

@vorot93
Copy link
Author

vorot93 commented May 22, 2020

By "Option B", do you mean option 2?

Yes

@vorot93 vorot93 force-pushed the vorot93/eip-2565b branch 2 times, most recently from 7b4b04e to d2123e3 Compare June 12, 2020 18:20
Copy link

@ineffectualproperty ineffectualproperty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided some suggested changes given the modifications to EIP-2565

"info": "EIP 2565 transition at block X",
"price": {
"modexp2": {
"divisor": 20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"divisor": 20
"divisor": 3

Self::mult_complexity
};

let (gas, overflow) = (complexity_formula)(m).overflowing_mul(max(adjusted_exp_len, 1));
if overflow {
return U256::max_value();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(gas / self.divisor as u64).into() --> min((gas / self.divisor as u64).into(),100)

This sets a minimum gas price of 100

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is a couple rows down but it wouldn't let me comment on it for some reason

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EIP has been updated to set the minimum gas price at 200

Pricing::Modexp(ModexpPricer {
divisor: if exp.divisor == 0 {
warn!(target: "builtin", "Zero modexp divisor specified. Falling back to default: 10.");
10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
10
3

@ineffectualproperty
Copy link

EIP has been updated in this PR: ethereum/EIPs#2892

@@ -219,6 +226,10 @@ impl ModexpPricer {
x => (x * x) / 16 + 480 * x - 199_680,
}
}

fn mult_complexity_new(x: u64) -> u64 {
((x / 64) + if x % 64 == 0 { 0 } else { 1 }) ^ 2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace 64 with 8 to represent limbs in bytes rather than bits per most recent EIP update

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants